Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pallet-xcm: Deprecate execute and send in favor of execute_blob and send_blob #3749

Merged
merged 53 commits into from
Mar 27, 2024
Merged

pallet-xcm: Deprecate execute and send in favor of execute_blob and send_blob #3749

merged 53 commits into from
Mar 27, 2024

Conversation

franciscoaguirre
Copy link
Contributor

@franciscoaguirre franciscoaguirre commented Mar 19, 2024 •
edited
Loading

execute and send try to decode the xcm in the parameters before reaching the filter line.
The new extrinsics decode only after the filter line.
These should be used instead of the old ones.

TODO

This would make writing XCMs in PJS Apps more difficult, but here's the fix for that: polkadot-js/apps#10350.
Already deployed! https://polkadot.js.org/apps/#/utilities/xcm

Supersedes #1798

@franciscoaguirre franciscoaguirre mentioned this pull request Mar 19, 2024
Closed
@franciscoaguirre
Copy link
Contributor Author

@pgherveou Could you take a look at ExecuteController and SendController and make sure the changes make sense? I don't want to break anything on the contracts side

pgherveou
pgherveou reviewed Mar 19, 2024
View reviewed changes
polkadot/xcm/xcm-builder/src/controller.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-builder/src/controller.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
@pgherveou
Copy link
Contributor

@pgherveou Could you take a look at ExecuteController and SendController and make sure the changes make sense? I don't want to break anything on the contracts side

this should work, I am not too sure where your new MaxXcmEncodedSize is defined though. If that helps we can also not add the legacy function to the traits since at that point the contract implementation is still unstable and ok to change.

@franciscoaguirre
Copy link
Contributor Author

I'll put only the new function in the traits then and ping you later to check everything would be fine with contracts.

@franciscoaguirre @pgherveou
Update polkadot/xcm/pallet-xcm/src/lib.rs
d2359c6 
Co-authored-by: PG Herveou <pgherveou@gmail.com>
@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Mar 20, 2024
@acatangiu
Copy link
Contributor

New PJS utility to allow same UX for these new extrinsics is already available, thanks to @serban300 👍
https://polkadot.js.org/apps/#/utilities/xcm

@franciscoaguirre franciscoaguirre marked this pull request as ready for review March 20, 2024 16:52
@franciscoaguirre franciscoaguirre requested a review from a team as a code owner March 20, 2024 16:52
@franciscoaguirre franciscoaguirre mentioned this pull request Mar 20, 2024
Open
7 tasks
acatangiu
acatangiu reviewed Mar 20, 2024
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! waiting for TODOs to be finished before approving.

polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/runtime/westend/src/weights/pallet_xcm.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
@franciscoaguirre franciscoaguirre requested a review from athei as a code owner March 20, 2024 20:00
@pgherveou
Copy link
Contributor

pgherveou commented Mar 21, 2024 •
edited
Loading

Can you tell a bit more about the motivations

execute and send try to decode the xcm in the parameters before reaching the filter line.

The new interface is a bit awkward, the caller needs to create an XCM, then encode it, so that it's later decoded again and processed inside the pallet.

Couldn't we just get away with an extra encoded_size() check on the passed message to see if it fits the constraint?

@franciscoaguirre
Copy link
Contributor Author

franciscoaguirre commented Mar 21, 2024 •
edited
Loading

You only encode and then decode because you're writing the code in rust, but normal users would already have to encode their XCM.
Taking it encoded sounds like a good pattern, and it only requires .encode(), and an optional .try_into() and error handling if it's too big, in rust code.

@franciscoaguirre
Copy link
Contributor Author

bot help

@acatangiu acatangiu enabled auto-merge March 25, 2024 17:46
@acatangiu
Copy link
Contributor

@pgherveou @bkontur this is ready, waiting for final review

@acatangiu acatangiu added this pull request to the merge queue Mar 27, 2024
Merged via the queue into master with commit feee773 Mar 27, 2024
128 of 134 checks passed
@acatangiu acatangiu deleted the xcm-execute-blob branch March 27, 2024 08:55
@pgherveou pgherveou mentioned this pull request Apr 9, 2024
Merged
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
@franciscoaguirre @pgherveou
@acatangiu @dharjeezy
pallet-xcm: Deprecate execute and send in favor of execute_blob…
5cc12f1 
… and `send_blob` (paritytech#3749)

`execute` and `send` try to decode the xcm in the parameters before
reaching the filter line.
The new extrinsics decode only after the filter line.
These should be used instead of the old ones.

## TODO
- [x] Tests
- [x] Generate weights
- [x] Deprecation issue ->
paritytech#3771
- [x] PRDoc
- [x] Handle error in pallet-contracts

This would make writing XCMs in PJS Apps more difficult, but here's the
fix for that: polkadot-js/apps#10350.
Already deployed! https://polkadot.js.org/apps/#/utilities/xcm

Supersedes paritytech#1798

---------

Co-authored-by: PG Herveou <pgherveou@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
This was referenced Apr 12, 2024
Open
Closed
franciscoaguirre added a commit that referenced this pull request Apr 24, 2024
@franciscoaguirre
Revert "pallet-xcm: Deprecate execute and send in favor of `execu…
8c41daa 
…te_blob` and `send_blob` (#3749)"

This reverts commit feee773.
@franciscoaguirre franciscoaguirre mentioned this pull request Apr 24, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 24, 2024
@franciscoaguirre @acatangiu @Bullrich
Revert execute_blob and send_blob (#4266)
4f3d43a 
Revert "pallet-xcm: Deprecate `execute` and `send` in favor of
`execute_blob` and `send_blob` (#3749)"

This reverts commit feee773.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Javier Bullrich <javier@bullrich.dev>
franciscoaguirre added a commit that referenced this pull request Apr 25, 2024
@franciscoaguirre @acatangiu @Bullrich
Revert execute_blob and send_blob (#4266)
c9633fe 
Revert "pallet-xcm: Deprecate `execute` and `send` in favor of
`execute_blob` and `send_blob` (#3749)"

This reverts commit feee773.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Javier Bullrich <javier@bullrich.dev>
@franciscoaguirre franciscoaguirre mentioned this pull request Apr 25, 2024
Merged
acatangiu added a commit that referenced this pull request Apr 29, 2024
@franciscoaguirre @acatangiu @Bullrich
Revert execute_blob and send_blob (#4266) (#4290)
8568329 
Revert "pallet-xcm: Deprecate `execute` and `send` in favor of
`execute_blob` and `send_blob` (#3749)"

This reverts commit feee773.

---------

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Javier Bullrich <javier@bullrich.dev>
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
@ahmadkaouk ahmadkaouk mentioned this pull request Jun 20, 2024
Merged
@chungquantin chungquantin mentioned this pull request Aug 13, 2024
Merged
@TDemeco TDemeco mentioned this pull request Sep 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@pgherveou pgherveou pgherveou approved these changes

@athei athei Awaiting requested review from athei

@bkontur bkontur Awaiting requested review from bkontur

Assignees
No one assigned
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@franciscoaguirre @pgherveou @acatangiu @paritytech-cicd-pr